Health status worker metrics improvements #10442
Conversation
packages/twenty-server/src/engine/core-modules/message-queue/drivers/bullmq.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/message-queue/drivers/bullmq.driver.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/health/indicators/worker.health.ts
Outdated
Show resolved
Hide resolved
ehconitin
force-pushed
the
health-status-worker-metrics-improvements
branch
from
b322619 to
67808e6
Compare
…r-metrics-improvements
…r-metrics-improvements
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR enhances worker metrics visualization in the admin panel health status section, adding time-series graphs and improving loading states.
- Added new
WorkerMetricsGraphcomponent inhealth-status/components/WorkerMetricsGraph.tsxthat displays queue performance metrics over time with interactive time range selection - Added
getQueueMetricsGraphQL query inhealth-status/graphql/queries/getQueueMetrics.tsto fetch time-series metrics data for worker queues - Enhanced server-side metrics collection in
admin-panel-health.service.tswith support for different time ranges (4H, 12H, 1D, 7D) - Added proper loading states with new
SettingsAdminTabSkeletonLoadercomponent for better UX during data fetching - Improved queue health button styling with hover effects in
SettingsAdminQueueHealthButtons.tsx
Greptile: I've analyzed the changes in this PR which focuses on enhancing the worker metrics visualization and health status monitoring in the admin panel.
22 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
.../twenty-front/src/modules/settings/admin-panel/components/SettingsAdminTabSkeletonLoader.tsx
Show resolved
Hide resolved
...ront/src/modules/settings/admin-panel/health-status/components/SettingsAdminHealthStatus.tsx
Show resolved
Hide resolved
...rc/modules/settings/admin-panel/health-status/components/SettingsAdminQueueHealthButtons.tsx
Show resolved
Hide resolved
| `} |
There was a problem hiding this comment.
style: Unnecessary whitespace in the template literal.
| it('should return metrics data for a queue', async () => { | ||
| const mockCompletedMetrics = { data: [10, 20, 30] }; | ||
| const mockFailedMetrics = { data: [1, 2, 3] }; | ||
|
|
||
| mockQueue.getMetrics | ||
| .mockResolvedValueOnce(mockCompletedMetrics) | ||
| .mockResolvedValueOnce(mockFailedMetrics); | ||
|
|
||
| workerHealth.getQueueDetails.mockResolvedValue({ | ||
| queueName: 'test-queue', | ||
| workers: 1, | ||
| status: 'up', | ||
| metrics: { | ||
| active: 1, | ||
| completed: 30, | ||
| failed: 3, | ||
| waiting: 0, | ||
| delayed: 0, | ||
| failureRate: 9.1, | ||
| }, | ||
| }); | ||
|
|
||
| const result = await service.getQueueMetricsOverTime( | ||
| MessageQueue.messagingQueue, | ||
| '4H', | ||
| ); | ||
|
|
||
| expect(result).toMatchObject({ | ||
| queueName: MessageQueue.messagingQueue, | ||
| timeRange: '4H', | ||
| data: expect.arrayContaining([ | ||
| expect.objectContaining({ | ||
| id: 'Completed Jobs', | ||
| data: expect.any(Array), | ||
| }), | ||
| expect.objectContaining({ | ||
| id: 'Failed Jobs', | ||
| data: expect.any(Array), | ||
| }), | ||
| ]), | ||
| }); |
There was a problem hiding this comment.
style: The test verifies the structure of the result but doesn't validate the actual data transformation logic. Consider adding assertions to verify that the metrics data from the mock is correctly transformed into the expected time series format.
| it('should handle empty metrics data', async () => { | ||
| mockQueue.getMetrics | ||
| .mockResolvedValueOnce({ data: [] }) | ||
| .mockResolvedValueOnce({ data: [] }); | ||
|
|
||
| workerHealth.getQueueDetails.mockResolvedValue(null); | ||
|
|
||
| const result = await service.getQueueMetricsOverTime( | ||
| MessageQueue.messagingQueue, | ||
| '4H', | ||
| ); | ||
|
|
||
| expect(result.data).toHaveLength(2); | ||
| expect(result.data[0].data).toHaveLength(48); // Default points for 4H | ||
| expect(result.data[1].data).toHaveLength(48); | ||
| }); |
There was a problem hiding this comment.
style: Test confirms the array lengths but doesn't verify the timestamps are correctly generated. Consider adding assertions to check the timestamp values match the expected pattern for the 4H timeframe.
| workers.length > 0 | ||
| ? failureRate > METRICS_FAILURE_RATE_THRESHOLD | ||
| ? 'down' | ||
| : 'up' | ||
| : 'down', |
There was a problem hiding this comment.
logic: Redundant condition check. Line 97 checks workers.length > 0 but we're already inside a block (line 64) that only executes when workers.length > 0.
| this.logger.error( | ||
| `Error getting queue details for ${queueName}: ${error.message}`, | ||
| ); | ||
| await queue.close(); |
There was a problem hiding this comment.
style: Queue close attempt after error might not execute if the error happens during queue.close() itself. Consider using try/finally to ensure cleanup.
No description provided.